Skip to content

Merge multifundchannel and fundchannel plugins, make fundchannel a thin wrapper around multifundchannel - #3769

Merged
rustyrussell merged 3 commits into
ElementsProject:masterfrom
ZmnSCPxj:multi-fundchannel-merge
Sep 10, 2020
Merged

Merge multifundchannel and fundchannel plugins, make fundchannel a thin wrapper around multifundchannel#3769
rustyrussell merged 3 commits into
ElementsProject:masterfrom
ZmnSCPxj:multi-fundchannel-merge

Conversation

@ZmnSCPxj

Copy link
Copy Markdown
Contributor

Requres #3763

@ZmnSCPxj
ZmnSCPxj requested a review from cdecker as a code owner June 15, 2020 08:54
@ZmnSCPxj
ZmnSCPxj force-pushed the multi-fundchannel-merge branch from 119acbb to 5ee7118 Compare June 16, 2020 04:59
@ZmnSCPxj

Copy link
Copy Markdown
Contributor Author

Update based on new #3763

@ZmnSCPxj
ZmnSCPxj force-pushed the multi-fundchannel-merge branch from 5ee7118 to f201927 Compare June 24, 2020 03:38
@ZmnSCPxj

Copy link
Copy Markdown
Contributor Author

Updated based on latest #3763

@ZmnSCPxj
ZmnSCPxj force-pushed the multi-fundchannel-merge branch 3 times, most recently from a1ff840 to 3be3119 Compare June 25, 2020 03:00
@cdecker

cdecker commented Jun 25, 2020

Copy link
Copy Markdown
Member

Blocked by #3763

@ZmnSCPxj

Copy link
Copy Markdown
Contributor Author

Rebased, also reimplemented the wrapper.

@ZmnSCPxj
ZmnSCPxj force-pushed the multi-fundchannel-merge branch 9 times, most recently from ee49371 to f806c47 Compare August 22, 2020 14:12
@ZmnSCPxj ZmnSCPxj added this to the v0.9.1 milestone Aug 23, 2020
Comment thread tests/test_closing.py
# funding tx.
addr = l1.rpc.call('dev-listaddrs', [keyidx + 3])['addresses'][-1]
addr = l1.rpc.call('dev-listaddrs', [keyidx + 2])['addresses'][-1]
# the above used to be keyidx + 3, but that was when `fundchannel`

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this comment is unnecessary, as the commit history will preserve the deleted comment + value.

channel_id = ok ? json_get_member(buf, channel_ids_obj, "channel_id") : NULL;
ok = ok && channel_id;
outnum = ok ? json_get_member(buf, channel_ids_obj, "outnum") : NULL;
ok = ok && outnum;

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i hate this. just return an error at the point which it fails.


if (!ok)
plugin_err(cmd->plugin,
"Unexpected result from nultifundchannel: %.*s",

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
"Unexpected result from nultifundchannel: %.*s",
"Unexpected result from multifundchannel: %.*s",

@niftynei

Copy link
Copy Markdown
Collaborator

cool, nice to see the code get consolidated

@niftynei niftynei closed this Aug 27, 2020
@niftynei niftynei reopened this Aug 27, 2020

@rustyrussell rustyrussell left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Other than minor cleanup suggested by @niftynei looks good.

Comment thread plugins/.gitignore
pay
multifundchannel
pay
spenderp

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

spenderp?

Comment thread common/json.c Outdated
json_to_int(buffer, tok, &i);
json_add_num(result, fieldname, i);
}
} else if (buffer[tok->start] == 't' || buffer[tok->start] == 'T')

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Garbage in, garbage out.

If they feed in an invalid primitive, we should either pass it through, or get upset with the passer.

But I can't understand why json_add_tok exists. Why are we trying to normalize tokens? We should just append them as we're asked to.

@darosior ?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But I can't understand why json_add_tok exists. Why are we trying to normalize tokens? We should just append them as we're asked to.

Using json_add_member ? I don't recall exactly but it seems cleaner to use our already-present helpers.

Anyway, i 1) don't know how comes i only handled the num primitive and 2) am pretty sure Null False and True here and below are invalid JSON (maybe just use json_add_literal in the else branch).

@rustyrussell rustyrussell Sep 10, 2020

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It also turns strings into bools, etc. It's just weird.

I'm testing the simplest variant now:

void json_add_tok(struct json_stream *result, const char *fieldname,
                  const jsmntok_t *tok, const char *buffer)
{
	char *space;
	assert(tok->type != JSMN_UNDEFINED);

	space = json_member_direct(result, fieldname, json_tok_full_len(tok));
	memcpy(space, json_tok_full(buffer, tok), json_tok_full_len(tok));
}

I'm not sure why we were turning strings into bools, etc.

Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
…ultifundchannel.

Changelog-Changed: protocol: `fundchannel` now shuffles inputs and outputs, and no longer follows BIP69.
@rustyrussell
rustyrussell force-pushed the multi-fundchannel-merge branch from f806c47 to ce6f30b Compare September 10, 2020 04:50
@rustyrussell

Copy link
Copy Markdown
Contributor

Fairly simple rebase. Removed the json_add_tok nonsense in favor of a simple copy.

@rustyrussell
rustyrussell merged commit 9587425 into ElementsProject:master Sep 10, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants